Repository navigation
Conversation
MathiasVP
force-pushed
the
fix-gl-value-as-expr
branch
from
November 8, 2026 17:04
47a1463 to
4c0bb1d
Compare
30 tasks
MathiasVP
marked this pull request as ready for review
November 8, 2026 18:08
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The reference-conversion mapping can associate the same qualifier with both the reference address and the object, causing queries to select both instead of a single intended node.
3 open issues
What changed in this PR
Corrects C++ expression-to-data-flow-node mapping for glvalue member-call qualifiers, distinguishing object values from their addresses. This supports more precise standard-library modeling while retaining the existing pointer/pointee taint rule.
Changes:
- Maps eligible glvalue qualifiers to indirect nodes while preserving temporary-materialization handling.
- Updates expected node and path labels affected by the mapping change.
| File | Description |
|---|---|
cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected |
Updates qualifier labels in XXE query paths. |
cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected |
Updates qualifier labels in field-flow paths. |
cpp/ql/test/library-tests/dataflow/external-models/steps.expected |
Updates labels for modeled data-flow steps. |
cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/ExprNodes.qll |
Adds glvalue qualifier mapping logic and explanatory comments. |
Review effort: Max (auto)
…f reference-related ones that currently causes duplicated results.
…in the convertion chain is a glvalue expression.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


This PR fixes a special case in the
Expr<->DataFlow::Nodemapping: When the qualifier instruction is a glvalue the resultingDataFlow::Nodefor whichasExprequals the qualifier expression was theInstructionNodefor the instruction, whereas it should be theIndirectInstructionNode.I noticed this problem in #22939 where we lost a result that I added as a testcase in #22990. It is essentially this situation:
On
mainit currently “works” because taint-tracking has this ad-hoc rule which conflates pointers and their indirection:codeql/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/TaintTrackingUtil.qll
Lines 212 to 229 in 682545f
I want to get rid of this disjunct at some point, but until we fix some more pointer/pointee conflation it has to stay (stay tuned for another upcoming PR!).
On
mainthis is the path we get for that test with debug-printing enabled:the
r153_3instruction is a glvalue-producing instruction which computes a pointer to thepasswordvariable. So thec_strstep steps from aglvalue<std::string>expression (the qualifier glvalue instruction) to aconst charexpression (the indirection of the resultingconst char*expression).When I disable the conflation rule and runs the query on this database again we get:
That is, we now start at the indirection of the qualifier (i.e., a
std::stringexpression) and thec_strmodel step gets us to aconst charexpression (the indirection of the resultingconst char*expression), as expected.Because of the above conflation there shouldn’t really be any changes to any results. However, since
toStringis defined in terms of which expressions a node maps to, there are lots of changes to .expected files.DCA was uneventful. There's a small change in a Coding Standards query's
.expectedfile that needs to be accepted. See the backlinked PR